Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: add MVCC-compliant AddSSTable variant #72085

Merged
merged 4 commits into from
Nov 25, 2021

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Oct 28, 2021

storage: tweak ScanIntents()

This renames ScanIntents from ScanSeparatedIntents, and adds a
context parameter. Callers have been updated to pass
storage.mvcc.max_intents_per_error for the intent limit, as is done
elsewhere, and targetBytes is passed as 0 for now (no limit) since
other intent collectors currently don't use a byte limit.

Touches #72563.

Release note: None

roachpb: assert request flag combinations

Request flags have implicit dependencies and incompatibilities (e.g.
isLocking implies isTxn). However, these were never checked and
developers were expected to satisfy them manually, which is error-prone.

This patch adds TestFlagCombinations that checks these dependencies
and incompatibilities, based on flagDependencies and flagExclusions
maps which encodes them.

It also adds a new flag type for flags, renames skipLeaseCheck to
skipsLeaseCheck, and adds isAlone for CheckConsistencyRequest.

Release note: None

roachpb: add appliesTSCache request flag

Previously, isIntentWrite determined whether a request checked the
timestamp cache and possibly pushed its timestamp. However, some
requests may want to check the timestamp cache without writing intents,
notably an MVCC-compliant AddSSTable request.

This patch introduces a new flag appliesTSCache, and uses it as a
condition for applying the timestamp cache to the request.

Release note: None

kvserver: add MVCC-compliant AddSSTable variant

AddSSTable does not comply with MVCC, the timestamp cache, nor the
closed timestamp, since the SST MVCC timestamps are written exactly as
given and thus can rewrite history.

This patch adds three new parameters that can make AddSSTable fully
MVCC-compliant, with a corresponding MVCCAddSSTable version gate:

  • WriteAtRequestTimestamp: rewrites the MVCC timestamps to the request
    timestamp, complying with the timestamp cache and closed timestamp.

  • DisallowConflicts: checks for any conflicting keys and intents at or
    above the SST's MVCC timestamps, complying with MVCC.

  • DisallowShadowingBelow: implies DisallowConflicts, and also errors
    if shadowing visible keys (but not tombstones). Unlike the existing
    DisallowShadowing, this allows shadowing existing keys above the given
    timestamp if the new key has the same value as the existing one, and
    also allows idempotent writes at or above the given timestamp.

The existing DisallowShadowing parameter implies DisallowConflicts,
and also errors on any existing visible keys below the SST key's
timestamp (but not tombstones). It no longer allows replacing a
tombstone with a value at the exact same timestamp.

Additionally, even blind AddSSTable requests that do not check for
conflicts now take out lock spans and scan for existing intents,
returning a WriteIntentError to resolve them. This should be cheap
in the common case, since the caller is expected to ensure there are no
concurrent writes over the span, and so there should be no or few
intents.

The WriteAtRequestTimestamp SST rewrite implementation here is correct
but slow. Optimizations will be explored later.

Resolves #70422.

Release note: None

@erikgrinaker erikgrinaker self-assigned this Oct 28, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

pkg/kv/kvserver/batcheval/cmd_add_sstable.go Outdated Show resolved Hide resolved
pkg/roachpb/api.proto Outdated Show resolved Hide resolved
pkg/roachpb/api.proto Outdated Show resolved Hide resolved
pkg/kv/kvserver/batcheval/cmd_add_sstable.go Outdated Show resolved Hide resolved
@erikgrinaker erikgrinaker force-pushed the addsstable-mvcc branch 3 times, most recently from 9b73a35 to 768d3b6 Compare October 29, 2021 11:36
@erikgrinaker erikgrinaker force-pushed the addsstable-mvcc branch 11 times, most recently from 9995d6f to 8bb7a20 Compare November 4, 2021 15:49
@erikgrinaker erikgrinaker force-pushed the addsstable-mvcc branch 4 times, most recently from 6f275ce to e75afdc Compare November 8, 2021 19:27
@erikgrinaker erikgrinaker removed the request for review from jbowens November 8, 2021 19:28
Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @andreimatei, @dt, @jbowens, @sumeerbhola, and @tbg)


pkg/roachpb/api.go, line 95 at r23 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Very nice!
How about

isAdmin -> isAlone
skipsLeaseCheck -> isAlone

and of course we could entertain

skipsLeaseCheck -> !isIntentWrite
and probably a lot more of that kind

but maybe that's past the scope of this PR, very happy about the unit test already.

Also I don't understand isPrefix, can you add more words while you're here?

I wish we just had, like 12, APIs for all of these requests instead of jamming them all into batches

Sure, done. I'm not familiar with a lot of the interactions here, but at least we have the infra in place to add them.

@erikgrinaker erikgrinaker force-pushed the addsstable-mvcc branch 5 times, most recently from 91c434b to 2ddd715 Compare November 13, 2021 09:58
@erikgrinaker erikgrinaker removed the request for review from andreimatei November 14, 2021 10:39
@tbg tbg removed their request for review November 16, 2021 09:43
@tbg
Copy link
Member

tbg commented Nov 16, 2021

Looks good for the KV side, so I think we're mostly waiting for @sumeerbhola at this point.

@tbg tbg removed the request for review from aliher1911 November 16, 2021 10:00
@erikgrinaker
Copy link
Contributor Author

I opened #73047 to track the possibility of making blind AddSSTable writes without DisallowConflicts scans completely safe by writing at MVCC timestamps that are known to be unused.

@sumeerbhola
Copy link
Collaborator

Didn't realize this was waiting on me. I was happy after my last comments were addressed.

@erikgrinaker
Copy link
Contributor Author

bors r=dt,tbg,sumeerbhola,nvanbenschoten

TFTRs!

@craig
Copy link
Contributor

craig bot commented Nov 25, 2021

Merge conflict.

This renames `ScanIntents` from `ScanSeparatedIntents`, and adds a
context parameter. Callers have been updated to pass
`storage.mvcc.max_intents_per_error` for the intent limit, as is done
elsewhere, and `targetBytes` is passed as 0 for now (no limit) since
other intent collectors currently don't use a byte limit.

Release note: None
Request flags have implicit dependencies and incompatibilities (e.g.
`isLocking` implies `isTxn`). However, these were never checked and
developers were expected to satisfy them manually, which is error-prone.

This patch adds `TestFlagCombinations` that checks these dependencies
and incompatibilities, based on `flagDependencies` and `flagExclusions`
maps which encodes them.

It also adds a new `flag` type for flags, renames `skipLeaseCheck` to
`skipsLeaseCheck`, and adds `isAlone` for `CheckConsistencyRequest`.

Release note: None
Previously, `isIntentWrite` determined whether a request checked the
timestamp cache and possibly pushed its timestamp. However, some
requests may want to check the timestamp cache without writing intents,
notably an MVCC-compliant `AddSSTable` request.

This patch introduces a new flag `appliesTSCache`, and uses it as a
condition for applying the timestamp cache to the request.

Release note: None
`AddSSTable` does not comply with MVCC, the timestamp cache, nor the
closed timestamp, since the SST MVCC timestamps are written exactly as
given and thus can rewrite history.

This patch adds three new parameters that can make `AddSSTable` fully
MVCC-compliant, with a corresponding `MVCCAddSSTable` version gate:

* `WriteAtRequestTimestamp`: rewrites the MVCC timestamps to the request
  timestamp, complying with the timestamp cache and closed timestamp.

* `DisallowConflicts`: checks for any conflicting keys and intents at or
  above the SST's MVCC timestamps, complying with MVCC.

* `DisallowShadowingBelow`: implies `DisallowConflicts`, and also errors
  if shadowing visible keys (but not tombstones). Unlike the existing
  `DisallowShadowing`, this allows shadowing existing keys above the given
  timestamp if the new key has the same value as the existing one, and
  also allows idempotent writes at or above the given timestamp.

The existing `DisallowShadowing` parameter implies `DisallowConflicts`,
and also errors on any existing visible keys below the SST key's
timestamp (but not tombstones). It no longer allows replacing a
tombstone with a value at the exact same timestamp.

Additionally, even blind `AddSSTable` requests that do not check for
conflicts now take out lock spans and scan for existing intents,
returning a `WriteIntentError` to resolve them. This should be cheap
in the common case, since the caller is expected to ensure there are no
concurrent writes over the span, and so there should be no or few
intents.

The `WriteAtRequestTimestamp` SST rewrite implementation here is correct
but slow. Optimizations will be explored later.

Release note: None
@erikgrinaker
Copy link
Contributor Author

bors r=dt,tbg,sumeerbhola,nvanbenschoten

@craig
Copy link
Contributor

craig bot commented Nov 25, 2021

Build succeeded:

@craig craig bot merged commit ebd9bed into cockroachdb:master Nov 25, 2021
@erikgrinaker erikgrinaker deleted the addsstable-mvcc branch November 25, 2021 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvserver: AddSSTable option to write at current timestamp
6 participants